Fix Pagination issue #55
Conversation
The newer versions of the Github API includes pagination. It means, without passing per_page, it's defaulting to the first 25 items. This causes issues if a user has more than 25 repositories. Or more than 25 open pull requests.
| @@ -55,6 +55,8 @@ class Client | |||
| 'api_limit' => 5000, | |||
| 'api_version' => 'beta', | |||
|
|
|||
| 'per_page' => 500, | |||
There was a problem hiding this comment.
Why not 30? That is the default from github.. and the limit is 100...
http://developer.github.com/v3/#pagination
There was a problem hiding this comment.
This should be 30 as it's default value or null as it's not required by GH.
|
this PR only works if you do not get more than the hard limit of the github api. which is as verschoof notes usually set at 100. and some projects could have more than even 500 items anyways, for example when fetching closed issues. note that there is also #49 and #53 that both are about pagination. in my opinion, we either need the option to make this library fetch all, no matter how many pages, or to somehow expose the paging to the user of the library. having to change the signature of every method that goes to the api client sounds painful, but maybe its really the only clean thing to do. but additionally we would still need to know if there are more pages. so maybe having a PagingClient and set limit and page on it before the request, then doing the request and then asking the client if there would be a next page is actually a better solution... @KnpLabs would be really cool if you could give some input as to what you prefer. it seems there are enough people around willing to contribute, but you should define the architecture you want. |
| @@ -119,10 +121,15 @@ public function addListener(ListenerInterface $listener) | |||
| */ | |||
| public function get($path, array $parameters = array(), array $headers = array()) | |||
| { | |||
| if(is_int($this->options['per_page']) && $this->options['per_page'] > 0){ | |||
There was a problem hiding this comment.
This should be different, this should go to setOption() not in get().
|
@justinburger fix by comments and fix a test, should be passed. @dbu, @verschoof, @justinburger, @stloyd let's discus bout pagination in a general, so put a page params under each method looks a quite painful and having global |
|
the main issue i see with the method parameters as in #53 is: how will a client do pagination? count the results and guess that if he got the maximum there could be a next page and try that? i think a neat solution could be to have a PagingClient that the user interacts with. that would solve the problem generically. on that client you could set the offset and limit for the next request, then have the request executed. ideally it would transparently get multiple pages if your limit is higher then the hard limit of github. if we want the #53 approach the client could just expose the fact if there is a next page, but otherwise you go through the normal methods. but all method signatures need to be updated. as the params are optional, i think we could do that without bc breaks for the user so its just some work, but no big issue for the users of this lib. |
|
@dbu We took exactly that approach, only we called it a "ResultPaginator" (@verschoof): https://github.com/Future500BV/php-github-api/commit/3683986823ed7995d0ff6f20074f821eba1dd050 We did not submit a pull request because it was a work-in-progress. Feel free to take a look, we'd be happy to finish this or at least bring it closer to being merged. |
|
Hi! Is pagination already implemented for Repos or Gists list? Can't see any information. |
|
This looks like a very cool approach to me that does not need to rewrite the whole api. Would be awesome if you could polish this a bit and do a PR! Not sure if i would find time anytime soon ----- Reply message ----- Future500BV@3683986 We did not submit a pull request because it was a work-in-progress. Feel free to take a look, we'd be happy to finish this or at least bring it closer to being merged. — |
|
Will do. Might be next week though. |
The newer versions of the Github API includes pagination. It means,
without passing per_page, it's defaulting to the first 25 items. This
causes issues if a user has more than 25 repositories. Or more than 25
open pull requests.
The solution I implemented is very simple, but it's been tested with large volume accounts for both repositories and pull requests.